-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Transmeta(TM) Crusoe(TM) CPU #238
base: master
Are you sure you want to change the base?
Conversation
cpufetch does not accept pull requests, see the contributing guidelines for details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very quick review, I'll have a look at the peak performance issue later.
bool SHA1; | ||
bool SHA2; | ||
bool CRC32; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about these 4 nit: remove space comments in cpu.h
: My editor removed the existing trailing spaces. I can revert those if you prefer to address them separately,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, that would be nice. I want to give this a format pass sometime soon.
@@ -692,6 +695,7 @@ struct topology* get_topology_info(struct cpuInfo* cpu, struct cache* cach, int | |||
|
|||
switch(cpu->cpu_vendor) { | |||
case CPU_VENDOR_INTEL: | |||
case CPU_VENDOR_TRANSMETA: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of this. From what I recall, get_topology_from_apic
is Intel only. It seems like it is getting the right number of cores but this is probably coming from get_topology_from_udev
. Can you confirm? Just run with --verbose
and it will show us how is it getting this information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Any help with finding out how to retrieve topology is appreciated. I believe we could also assume cores=1 on this platform.
[WARNING]: Can't read features information from cpuid (needed level is 0x00000007, max is 0x00000001)
[WARNING]: Can't read frequency information from cpuid (needed level is 0x00000016, max is 0x00000001). Using udev
[WARNING]: Can't read topology information from cpuid (needed level is 0x00000001, max is 0x00000001)
@@ -767,6 +771,32 @@ struct topology* get_topology_info(struct cpuInfo* cpu, struct cache* cach, int | |||
return topo; | |||
} | |||
|
|||
struct cache* get_cache_info_transmeta(struct cache* cach) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we use get_cache_info_general
. It does not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It failed to retrieve the cache size with that. I have eventually hardcoded the cache size to this CPU model.
I don't see why you get 0 MFLOP/s in peak performance. You will have to go to int64_t flops = topo->physical_cores * topo->sockets * (freq*1000000) * vpus; I suspect some of those values might be zero. |
Hey @Dr-Noob thanks a lot for the detailed review. Glad to see this is being considered. I'll be addressing your comments ASAP. |
@Dr-Noob I've fixed the style/dead code comments. About retreiving topology and cache information, I'm not sure if you want to provide some guidance for me to move ahead. I might be able to provide you temporary access to the machine if you think this might be quicker. :) |
also, let me know if you want me to rebase this on top of latest master |
Thank you for the feedback and the offer, but for now I think we should be good if you just do it in your machine 👍 My general guidance is to use cpuid whenever possible. This pdf should be used as a reference. You can also look at the cpufetch documentation (and more specifically, x86 docs) which explains how cpuid is used for various detections in both Intel and AMD. You can have this as an example and to understand how it is done in general and then have a look at the transmeta pdf to see the particular details of this vendor. |
I might commit some additional stuff in the upcoming days, so you could rebase just after this PR is ready to be merged. |
thanks for your patience. I've been a few days AFK, I'll resume on this during the weekend. |
I am aware of
contributing guidelines
1, but I thought this might be helpful if you don't have a Crusoe at hand.Initial support for Transmeta Crusoe CPU. Tested on TM5800 @ 1GHz.
Still trying to figure out how to read cache topology/set peak performance, but at least now my CPU is detected.
Upstream:
With this PR:
neofetch
, for reference: